Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding lily58pro and preonic keymap #10013

Merged
merged 24 commits into from
Sep 22, 2020
Merged

Adding lily58pro and preonic keymap #10013

merged 24 commits into from
Sep 22, 2020

Conversation

drasbeck
Copy link
Contributor

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drasbeck drasbeck changed the title Adding drasbeck keymap. Adding lily58pro keymap Aug 20, 2020
drashna
drashna requested changes Aug 23, 2020
View reviewed changes
keyboards/lily58/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/keymap.c
Comment on lines +35 to +38
QWERTY = SAFE_RANGE,
LOWER,
RAISE,
ADJUST,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting nit-pick

Suggested change
QWERTY = SAFE_RANGE,
LOWER,
RAISE,
ADJUST,
QWERTY = SAFE_RANGE,
LOWER,
RAISE,
ADJUST,

keyboards/lily58/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team August 29, 2020 03:31
@drashna drashna added the keymap label Aug 29, 2020
@drasbeck
Copy link
Contributor Author

drasbeck commented Sep 3, 2020 •
edited
Loading

@drashna thank you very much for all the feedback! I've tested your changes, and things seem to work.

The firmware is oddly larger after your suggestions are implemented. From 21028/28672 (73%, 7644 bytes free) to 21122/28672 (73%, 7550 bytes free). Is it the enum?

@drasbeck drasbeck changed the title Adding lily58pro keymap Adding lily58pro and preonic keymap Sep 3, 2020
@drashna drashna requested a review from a team September 6, 2020 21:28
@drashna
Copy link
Member

drashna commented Sep 6, 2020 •
edited
Loading

The firmware is oddly larger after your suggestions are implemented. From 21028/28672 (73%, 7644 bytes free) to 21122/28672 (73%, 7550 bytes free). Is it the enum?

It's possible, that would be ... very odd. If you updated your repo, that could be why, though

@drasbeck
Copy link
Contributor Author

drasbeck commented Sep 9, 2020

It's possible, that would be ... very odd. If you updated your repo, that could be why, though

Agreed, that is a lot of space for one enum. I just found it odd, that mostly removing lines (as was your suggestions), would result in a larger firmware. I don't remember updating, you mean pulling in from origin right? Perhaps I did after all. ;)

noroadsleft
noroadsleft requested changes Sep 19, 2020
View reviewed changes
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one thing.

keyboards/preonic/keymaps/drasbeck/readme.md Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team September 20, 2020 00:49
fauxpark
fauxpark requested changes Sep 20, 2020
View reviewed changes
keyboards/lily58/keymaps/drasbeck/rules.mk Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/rules.mk Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/rules.mk Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/drasbeck/rules.mk Outdated Show resolved Hide resolved
keyboards/preonic/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/preonic/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/preonic/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/preonic/keymaps/drasbeck/keymap.c Outdated Show resolved Hide resolved
keyboards/preonic/keymaps/drasbeck/rules.mk Outdated Show resolved Hide resolved
drasbeck and others added 8 commits September 21, 2020 22:23
@drasbeck @fauxpark
Update keyboards/preonic/keymaps/drasbeck/keymap.c
bae1938 
Co-authored-by: Ryan <[email protected]>
@drasbeck @fauxpark
Update keyboards/preonic/keymaps/drasbeck/keymap.c
608bc9e 
Co-authored-by: Ryan <[email protected]>
@drasbeck @fauxpark
Update keyboards/preonic/keymaps/drasbeck/keymap.c
af022d6 
Co-authored-by: Ryan <[email protected]>
@drasbeck @fauxpark
Update keyboards/preonic/keymaps/drasbeck/keymap.c
49f4f34 
Co-authored-by: Ryan <[email protected]>
@drasbeck @fauxpark
Update keyboards/lily58/keymaps/drasbeck/rules.mk
d72e2cc 
Co-authored-by: Ryan <[email protected]>
@drasbeck @fauxpark
Update keyboards/lily58/keymaps/drasbeck/rules.mk
f4a8959 
Co-authored-by: Ryan <[email protected]>
@drasbeck @fauxpark
Update keyboards/lily58/keymaps/drasbeck/rules.mk
fb6ef51 
Co-authored-by: Ryan <[email protected]>
@drasbeck @fauxpark
Update keyboards/preonic/keymaps/drasbeck/rules.mk
7555644 
Co-authored-by: Ryan <[email protected]>
@drasbeck
Copy link
Contributor Author

@fauxpark @noroadsleft thanks for reviewing my keymaps. All change requests committed.

noroadsleft
noroadsleft approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@noroadsleft noroadsleft merged commit 2fbf68f into qmk:master Sep 22, 2020
@noroadsleft
Copy link
Member

For future reference, we recommend against committing to your master branch as you've done here, because pull requests from modified master branches can make it more difficult to keep your QMK fork updated. It is highly recommended for QMK development – regardless of what is being done or where – to keep your master updated, but NEVER commit to it. Instead, do all your changes in a branch (branches are basically free in Git) and issue PRs from your branches when you're developing.

There are instructions on how to keep your fork updated here:

Best Practices: Your Fork's Master: Update Often, Commit Never

Fixing Your Branch will walk you through fixing up your master branch moving forward. If you need any help with this just ask.

Thanks for contributing!

rgoulter pushed a commit to rgoulter/qmk_firmware that referenced this pull request Oct 4, 2020
@drasbeck @rgoulter
[Keymap] Adding drasbeck keymaps for lily58pro and preonic (qmk#10013)
db7b867 
* added danskish keymap

* Added readme.md

* Clean up in keymap.c

* Added license

* Changed name of keymap

* adjusting tapping term

* added encoder functionality

* housekeeping

* layouts resemble the layout now

* implemented suggestions from drashna

* added keymap for preonic rev3

* added default layer to readme.md

* removed backslashes

* Update keyboards/lily58/keymaps/drasbeck/keymap.c

* Update keyboards/preonic/keymaps/drasbeck/readme.md

* Update keyboards/lily58/keymaps/drasbeck/rules.mk

* Update keyboards/preonic/keymaps/drasbeck/keymap.c

* Update keyboards/preonic/keymaps/drasbeck/keymap.c

* Update keyboards/preonic/keymaps/drasbeck/keymap.c

* Update keyboards/preonic/keymaps/drasbeck/keymap.c

* Update keyboards/lily58/keymaps/drasbeck/rules.mk

* Update keyboards/lily58/keymaps/drasbeck/rules.mk

* Update keyboards/lily58/keymaps/drasbeck/rules.mk

* Update keyboards/preonic/keymaps/drasbeck/rules.mk
kjganz pushed a commit to kjganz/qmk_firmware that referenced this pull request Oct 28, 2020
@drasbeck
[Keymap] Adding drasbeck keymaps for lily58pro and preonic (qmk#10013)
d9a5e2d 
* added danskish keymap

* Added readme.md

* Clean up in keymap.c

* Added license

* Changed name of keymap

* adjusting tapping term

* added encoder functionality

* housekeeping

* layouts resemble the layout now

* implemented suggestions from drashna

* added keymap for preonic rev3

* added default layer to readme.md

* removed backslashes

* Update keyboards/lily58/keymaps/drasbeck/keymap.c

* Update keyboards/preonic/keymaps/drasbeck/readme.md

* Update keyboards/lily58/keymaps/drasbeck/rules.mk

* Update keyboards/preonic/keymaps/drasbeck/keymap.c

* Update keyboards/preonic/keymaps/drasbeck/keymap.c

* Update keyboards/preonic/keymaps/drasbeck/keymap.c

* Update keyboards/preonic/keymaps/drasbeck/keymap.c

* Update keyboards/lily58/keymaps/drasbeck/rules.mk

* Update keyboards/lily58/keymaps/drasbeck/rules.mk

* Update keyboards/lily58/keymaps/drasbeck/rules.mk

* Update keyboards/preonic/keymaps/drasbeck/rules.mk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fauxpark fauxpark fauxpark requested changes

@noroadsleft noroadsleft noroadsleft approved these changes

@drashna drashna drashna approved these changes

Assignees
No one assigned
Labels
keymap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@drasbeck @drashna @noroadsleft @fauxpark